Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Swap from YAML Clap CLI to derived struct Clap CLI #161

Merged
merged 7 commits into from
Aug 23, 2023
Merged

Conversation

seblund
Copy link
Member

@seblund seblund commented Mar 30, 2023

YAML in Clap has been deprecated and we were two major versions behind. This PR changes the CLI to be derived from a struct and it is much cleaner IMO. We are now on the newest version of Clap which resolves #143.

Before, we had a very flat structure as we had no subcommands, e.g. you could technically supply a query, an input-folder, and give the -p argument to start the server at the same time as they were all optional.

I personally think this resulted in a very confusing user experience from the terminal, having all arguments be contextually optional is a bit strange as running a single query and starting a server are two very different. It would be nice if they are separate.

In this PR there are two subcommands serve and query:
image
This is nice because we can avoid a bunch of the optional arguments. E.g. if you are starting a server, you can only supply the ip, port and other relevant arguments, not an input folder and so on:
image
And we can provide contextual examples for the subcommands without overwhelming the user from the beginning:
image

If we prefer the flat structure of the CLI from before it is a very quick change, so discuss here what you prefer :)

@seblund seblund linked an issue Mar 30, 2023 that may be closed by this pull request
Base automatically changed from new_results_refactor to Ecdar-SW5/main April 6, 2023 15:32
Base automatically changed from Ecdar-SW5/main to main May 11, 2023 14:17
@andreaskrath
Copy link

I would suggest adding some test cases to the new CLI structure to ensure arguments are matches correctly.

The Parser trait from clap has a parse_from associated function which matches arguments from the supplied parameter; the parameter must implement the IntoIterator trait and its items must implement the Into<OsString> trait - which a string vector already does. The parse() associated function from the Parser trait uses this function underlying, and simply supplies the arguments via std::env::args_os().

As such, you can test the CLI structure by creating string or string slice vectors and use the parse_from associated function, to ensure correct argument matching.

Look at https://docs.rs/clap/latest/clap/trait.Parser.html for more info about the Parser trait.

Provided and example based on some of the screenshots you linked in your pull request.

#[test]
fn serve_command_with_t_and_c_flags() {
    // 0th argument does not matter, but it must be present
    let input_args = vec!["", "serve", "-t", "1", "-c", "50", "127.0.0.1:4242"];
    let args_matches = Args::parse_from(input_args);
    match args_matches {
        Args::Serve {
            endpoint,
            thread_count,
            cache_size,
        } => {
            assert_eq!(endpoint, "127.0.0.1:4242");
            assert_eq!(thread_count, 1);
            assert_eq!(cache_size, 50);
        },
        _ => unreachable!(),
    }
}

The primary purpose behind this would be to ensure the CLI is not mistakenly broken at some point, if further changes are made to it.

@t-lohse t-lohse merged commit 4b41c9d into main Aug 23, 2023
7 checks passed
@t-lohse t-lohse deleted the clap-derive branch August 23, 2023 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clap using deprecated yaml feature
3 participants